Skip to content

Bugfix/2568 output fenceposting#2575

Open
fluidnumericsJoe wants to merge 7 commits intomainfrom
bugfix/2568-output-fenceposting
Open

Bugfix/2568 output fenceposting#2575
fluidnumericsJoe wants to merge 7 commits intomainfrom
bugfix/2568-output-fenceposting

Conversation

@fluidnumericsJoe
Copy link
Copy Markdown
Contributor

AI Disclosure

  • [ X ] This PR contains AI-generated content.
    • [ X ] I have tested any AI-generated content in my PR.
    • [ X ] I take responsibility for any AI-generated content in my PR.
    • Describe how you used it (e.g., by pasting your prompt): After making changes to dump output after each positionupdate and add initial condition IO, I've used Claude Code to review Issue Particle positions in output are lagged by one dt relative to time labels #2568 with failing tests for context to suggest potential test failure resolution options.

In the previous version of parcels, particleset.execute() overshoots the
particle trajectories by exactly one time step while leading to repeated
initial condition output lagged by exactly one time step. This leads to
an inconsistency in the actual particle positions and those written to
files

In this updated approach, when an outputfile is provided, we write the
initial condition to file before the time loop. Then, inside the time
loop, the kernels are executed with particle position updated
immediately after all other kernels and just before file IO. This
corrects the inconsistency in the actual and reported time levels for
each particle state in the output.

Unfortunately this breaks a number of tests. The unit tests are checking
for incorrect values (lagged by exactly one time loop iteration..)
…ate after user kernels

This removes the `PositionUpdate` kernel from the list of kernels. This
change was made to fix `funcname` polution if the `test_kernel_merging`,
`test_kernel_from_list`, and `test_metadata`.
With the correction in place, the particle positions are now obtained by
1 less call to positionupdate (correctly); the values in the test output
for validation were based off the wrong number of iterations due to the
fenceposting bug we're trying to address.
…ion time

With the fence posting bugfix in place, the particleset execute call
updates the position once; previously, this happened twice (this was the
bug). This test failed because the particle didn't go out of bounds with
a single position update. Semantically, setting the runtime to 2 days,
achieved what was intended here (to get the particle out of bounds)
Copy link
Copy Markdown
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement! But it seems that this doesn't (yet) fix the ParticleFile? That now raises some unit test errors. Do you want me to help?

Comment on lines +223 to +224
# apply position/time update only to particles still in a normal state
# (particles that signalled Stop*/Delete/errors should not have time/position advanced)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So was this the fix to the bug in #2568? As simple as that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix was really to have explicit initial condition IO and file output after each call to positionupdate. The previous logic, which pre-pended positionupdate after the first call to kernel.execute() inadvertently wrote the initial condition twice, once for the initial time and once for the initial time + dt; this introduced the "off-by-dt" error in the particlefile output.


if pset._requires_prepended_positionupdate_kernel:
self.prepend_positionupdate_kernel()
self._position_update = self._make_position_update()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not directly call self._make_position_update()? Why (effectively) rename the function here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be much simpler than what I concocted.

pset = ParticleSet(fieldset, pclass=PeriodicParticle, lon=startlon, lat=[0.5, 0.5])
pset.execute([AdvectionEE, periodicBC], runtime=np.timedelta64(40, "s"), dt=np.timedelta64(1, "s"))
np.testing.assert_allclose(pset.total_dlon, 4.1, atol=1e-5)
np.testing.assert_allclose(pset.total_dlon, 4.0, atol=1e-5)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see that you also fixed all these 'bugs'!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the extra update call that was there internally, there was a ton of these assertions that were right for the wrong reasons :)

Comment on lines +22 to +25
pfile = parcels.ParticleFile(store="test.zarr", outputdt=dt)
pset.execute(integrator, runtime=T, dt=dt, output_file=pfile, verbose_progress=False)
expected_lon = 8.6
np.testing.assert_allclose(pset.lon, expected_lon, atol=1e-5)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the last locations in the output are not tested here. Also test that these are as expected?

@fluidnumericsJoe
Copy link
Copy Markdown
Contributor Author

Nice improvement! But it seems that this doesn't (yet) fix the ParticleFile? That now raises some unit test errors. Do you want me to help?

It'd be great to get your input on those. It looks like I need to have some awareness for particles being injected at different points in time into the simulation for the explicit initial condition write in particleset.execute()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

Particle positions in output are lagged by one dt relative to time labels

2 participants